websocket: avoid using Buffer.byteLength #3394
Conversation
lib/web/websocket/sender.js
Outdated
| @@ -92,12 +89,12 @@ function createFrame (data, hint) { | |||
| function toBuffer (data, hint) { | |||
| switch (hint) { | |||
| case sendHints.string: | |||
| return Buffer.from(data) | |||
| return typeof data === 'string' ? Buffer.from(data) : new Uint8Array(data.buffer, data.byteOffset, data.byteLength) | |||
There was a problem hiding this comment.
when can data not be a string?
There was a problem hiding this comment.
This PR provides a buffer instead of a string, so it may not be a string.
There was a problem hiding this comment.
so it won't be a string anymore, shouldn't sendHints.string be removed and have this case default to sendHints.typedArray?
There was a problem hiding this comment.
I can't do it because it's using sendHints.string to determine if it's a text frame or not.
There was a problem hiding this comment.
Or delete toBuffer.
diff --git a/lib/web/websocket/constants.js b/lib/web/websocket/constants.js
index 2019b5b6..ac1c6fe1 100644
--- a/lib/web/websocket/constants.js
+++ b/lib/web/websocket/constants.js
@@ -48,9 +48,8 @@ const emptyBuffer = Buffer.allocUnsafe(0)
const sendHints = {
string: 1,
- typedArray: 2,
- arrayBuffer: 3,
- blob: 4
+ binary: 2,
+ blob: 3
}
module.exports = {
diff --git a/lib/web/websocket/sender.js b/lib/web/websocket/sender.js
index 130024f2..10f206be 100644
--- a/lib/web/websocket/sender.js
+++ b/lib/web/websocket/sender.js
@@ -51,7 +51,7 @@ class SendQueue {
const node = {
promise: item.arrayBuffer().then((ab) => {
node.promise = null
- node.frame = createFrame(ab, hint)
+ node.frame = createFrame(new Uint8Array(ab), sendHints.binary)
}),
callback: cb,
frame: null
@@ -83,19 +83,7 @@ class SendQueue {
}
function createFrame (data, hint) {
- return new WebsocketFrameSend(toBuffer(data, hint)).createFrame(hint === sendHints.string ? opcodes.TEXT : opcodes.BINARY)
-}
-
-function toBuffer (data, hint) {
- switch (hint) {
- case sendHints.string:
- return typeof data === 'string' ? Buffer.from(data) : data
- case sendHints.arrayBuffer:
- case sendHints.blob:
- return new Uint8Array(data)
- case sendHints.typedArray:
- return new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
- }
+ return new WebsocketFrameSend(data).createFrame(hint === sendHints.string ? opcodes.TEXT : opcodes.BINARY)
}
module.exports = { SendQueue }
diff --git a/lib/web/websocket/websocket.js b/lib/web/websocket/websocket.js
index 9756c474..b664c2e3 100644
--- a/lib/web/websocket/websocket.js
+++ b/lib/web/websocket/websocket.js
@@ -253,10 +253,12 @@ class WebSocket extends EventTarget {
// increase the bufferedAmount attribute by the length of the
// ArrayBuffer in bytes.
- this.#bufferedAmount += data.byteLength
- this.#sendQueue.add(data, () => {
- this.#bufferedAmount -= data.byteLength
- }, sendHints.arrayBuffer)
+ const buffer = new Uint8Array(data)
+
+ this.#bufferedAmount += buffer.byteLength
+ this.#sendQueue.add(buffer, () => {
+ this.#bufferedAmount -= buffer.byteLength
+ }, sendHints.binary)
} else if (ArrayBuffer.isView(data)) {
// If the WebSocket connection is established, and the WebSocket
// closing handshake has not yet started, then the user agent must
@@ -270,10 +272,12 @@ class WebSocket extends EventTarget {
// not throw an exception must increase the bufferedAmount attribute
// by the length of data’s buffer in bytes.
- this.#bufferedAmount += data.byteLength
- this.#sendQueue.add(data, () => {
- this.#bufferedAmount -= data.byteLength
- }, sendHints.typedArray)
+ const buffer = new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
+
+ this.#bufferedAmount += buffer.byteLength
+ this.#sendQueue.add(buffer, () => {
+ this.#bufferedAmount -= buffer.byteLength
+ }, sendHints.binary)
} else if (isBlobLike(data)) {
// If the WebSocket connection is established, and the WebSocket
// closing handshake has not yet started, then the user agent must
There was a problem hiding this comment.
I can't do it because it's using sendHints.string to determine if it's a text frame or not.
Seems a little obvious, surprised I forgot that.
Or delete toBuffer.
I thought about it for a little bit; toBuffer should stay. I don't think there's a simple way of explaining my reasoning but I'll try. When talking about the WebSocket spec we are referring to two different specs: RFC 6455 and the WHATWG websocket spec. The latter is an abstraction on top of the rfc. As such, if you take a look at the whatwg spec, you'll notice that it hardly does anything, instead preferring to reference the rfc for the logic. That was my approach when creating WebSocket, to implement RFC 6455 and build the WebSocket client on top of that. The easiest example I can give is WebsocketStream, which I was planning on using the rfc 6455 parts for, but not the whatwg parts.
(new paragraph, finally)
I think the queue system makes more sense now, you pass in a <data type> and it works. No need to convert the data beforehand (the conversions we do are part of the whatwg spec). You'll see this with every other part of WebSocket; it delegates the logic to a lower-level, rather than handling it inside the client directly.
Once we start moving the logic away from the RFC parts, it'll inevitably make it harder to implement WebsocketStream. Actually, I suspect that I'd end up reverting the changes done here to implement it if it was changed to remove toBuffer.
I will agree that I have not done a great job at separating whatwg logic from the protocol logic. I have some ideas which I may explore once we land this.
There was a problem hiding this comment.
So... to get back to the original discussion, I have these ideas:
- rename
sendHints.stringtosendHints.textsince the type is no longer a string. - remove the
typeof data === 'string'check becausedatais no longer a string. If it can be a string, I think those sections should just wrap it in a Buffer.from(...) for now.
I appreciate the changes. I am more lenient with WebSocket changes because the whatwg spec doesn't do much and the rfc is for the protocol itself.
There was a problem hiding this comment.
Your reply gave me the following idea:
-
removewebidl.converters.WebSocketSendDataand add an asseertion function, e.g.,webidl.assetions.WebSocketSendData -
simplify by moving websocket send data conversion to SendQueue and adding a single function e.g.
SendQueue#write(data[, callback ]).
It also simplifies the implementation of #3395.
- Move close frame generation logic to a new function
tsctx
force-pushed
the
websocket/avoid-using-byteLength
branch
from
9a60ec0 to
8fb8f0f
Compare
small faster